🤖 Isaac Lab Review Bot — PR #5103

Summary

This PR addresses two visualizer bugs and expands test coverage with non-black-frame integration tests. The changes span 5 files:

1. simulation_context.py — Adds viz.step(0.0) when rendering is paused, keeping the visualizer event loop responsive so UI controls (e.g. "Resume Rendering") remain interactive.
2. setup.py — Adds munch as a dependency for the Newton visualizer.
3. test_simulation_context_visualizers.py — Adds a dedicated unit test for the rendering-paused fix and updates an existing test assertion.
4. kit_visualizer.py — Rewrites step() to use carb.settings directly (replacing get_settings_manager), and conditionally skips app.update() when playSimulations is already falsy. Rewrites is_training_paused() to use omni.timeline instead of the settings flag.
5. test_visualizer_smoke_logs.py — Adds ~174 lines of integration test infrastructure and two new parametrized tests (test_kit_visualizer_non_black_viewport_frame, test_newton_visualizer_non_black_viewer_frame) that load a Cartpole camera scene and assert non-black rendered output.

Design Assessment

Rendering-paused fix (simulation_context.py): ✅ Sound approach. Calling viz.step(0.0) with zero dt while rendering is paused keeps event loops responsive without advancing simulation time. The fix targets the root cause — previously the continue statement completely skipped the visualizer, leaving its UI unresponsive. The zero-dt convention is already established for the training-pause loop on the adjacent line.

Kit visualizer step() rewrite: ✅ Good improvement. Replacing get_settings_manager() with direct carb.settings access removes an unnecessary abstraction layer. The conditional should_restore logic avoids the previous bug where app.update() was unconditionally called even when simulations weren't playing, which caused the Play/Pause button to erroneously unpause the physics.

Kit visualizer is_training_paused() rewrite: ✅ Correct. Using omni.timeline transport state is semantically accurate — the old implementation checked the /app/player/playSimulations settings flag which could be transiently toggled by step() itself, creating false positives for training pause detection. Timeline transport state is the authoritative source.

Non-black-frame tests: ⚠️ Reasonable intent but CI shows these new tests are fragile in shared GPU CI environments. The CUDA OOM and illegal memory access errors causing test failures are likely caused by test process isolation issues (GPU state leaking between test cases), not by logic bugs in this PR.

Architecture Impact

• Low risk. The simulation_context.py change is a one-line addition inside an existing code path.
• The kit_visualizer.py changes replace one internal API (get_settings_manager) with another (carb.settings), reducing coupling. The timeline-based pause detection is more robust.
• Adding munch to setup.py is a new runtime dependency — this is appropriate if Newton visualizer requires it, but the version should ideally be pinned (see Finding #1).

Implementation Verdict

Approve with findings. The bug fixes are correct and well-targeted. The test infrastructure is solid. However, the new integration tests have CI reliability issues that should be addressed before merge.

Test Coverage

CI Status

Findings

Finding 1 — munch dependency is unpinned

File: source/isaaclab/setup.py, line 25
Severity: Low
Issue: munch is added without a version pin. Every other dependency in this section has an explicit version or minimum version constraint. An unpinned munch could cause unexpected breakage if a future major version introduces breaking API changes.
Suggestion: Pin to a minimum version, e.g. "munch>=4.0.0" or "munch==4.0.0".

Finding 2 — Kit step() skips app.update() when playSimulations is already falsy

File: source/isaaclab_visualizers/isaaclab_visualizers/kit/kit_visualizer.py, lines 108-119
Severity: Medium
Issue: The new logic only calls app.update() inside if should_restore, meaning when playSimulations is already False, app.update() is never called at all. The old code unconditionally called app.update() (after disabling playSimulations). This behavioral change means the Kit viewport will not pump UI events when simulation is already paused, which could prevent viewport refresh in paused-then-rendering scenarios.

# Current:
should_restore = bool(previous_play_simulations)
if should_restore:
    settings.set_bool(play_simulations_path, False)
    app.update()
if should_restore:
    settings.set_bool(play_simulations_path, True)

# Consider instead:
should_restore = bool(previous_play_simulations)
if should_restore:
    settings.set_bool(play_simulations_path, False)
app.update()  # Always pump UI events
if should_restore:
    settings.set_bool(play_simulations_path, True)

Alternatively, if the intent is specifically to skip app.update() when paused, document this behavior with a comment explaining why.

Finding 3 — New integration tests are not resilient to GPU state corruption from prior tests

File: source/isaaclab_visualizers/test/test_visualizer_smoke_logs.py
Severity: Medium
Issue: The CI failures show that test_newton_visualizer_non_black_viewer_frame[physx] and [newton] fail with torch.AcceleratorError: CUDA error: an illegal memory access during SimulationContext.__init__ — this is GPU state corruption carried over from the preceding test_kit_visualizer_non_black_viewport_frame[newton] failure (CUDA OOM in render product allocation). Since all tests run in a single process (required by Kit app lifetime), a CUDA error in one test poisons all subsequent GPU operations.
Suggestion: Consider adding pytest.mark.order to run the non-black frame tests last, or adding a CUDA device reset/sync check at the start of each test. Alternatively, mark these as @pytest.mark.xfail(reason="Requires dedicated GPU resources") until the CI environment is stabilized, so they don't block the PR pipeline.

Finding 4 — PR description mentions a known TODO that is not addressed

Severity: Info
Issue: The PR body states: "TODO: fix Unpause behavior, currently body transforms remain stale after Unpausing... recent regression". This indicates a known remaining bug in the Kit visualizer's pause/unpause lifecycle. This is fine as a known limitation, but consider filing a tracking issue so it doesn't get lost.

Review by 🤖 Isaac Lab Review Bot — 4a42c18b

🤖 Isaac Lab Review Bot — Follow-up

New commits (4a42c18→053adbd4): renamed camera_position/camera_target → eye/lookat across the codebase, simplified Kit step(), and merged develop.

One new issue found in the rename.

🤖 Isaac Lab Review Bot — PR #5103

Summary

This PR addresses multiple visualizer bugs (Newton pause button, Kit headless recording, rerun+newton launch, RTX stale images, wrist camera orientation), renames camera_position/camera_target to eye/lookat across the board, deprecates ViewerCfg overrides in favor of VisualizerCfg subclasses, and replaces the lightweight smoke tests with comprehensive cartpole-based integration tests that verify non-black frames and motion detection. The scope is significant — 24 files touching core simulation, env initialization, physics hooks, visualizer implementations, installation tooling, and test infrastructure.

Design Assessment

Mostly sound, with one architectural concern flagged by the maintainer.

The overall direction — dedicated visualizer cameras, deprecating ViewerCfg, decoupling Kit-specific logic from SimulationContext — is correct and moves toward cleaner separation. The after_visualizers_render() hook in PhysicsManager with the Kit-specific override in PhysxManager is a reasonable pattern to keep SimulationContext backend-agnostic.

However, has_active_visualizers() and can_render_rgb_array() in SimulationContext (simulation_context.py) directly query Isaac Sim settings paths (/isaaclab/visualizer/types). As maintainer @kellyguo11 noted, this introduces backend-specific coupling into what should be a backend-agnostic class. Consider moving these helpers to the PhysicsManager interface or making the setting path discovery pluggable.

Findings

🔴 Critical: _pending_camera_view not set in unit test helpers — CI failure — simulation_context.py:603 / test_simulation_context_visualizers.py

The _pending_camera_view attribute is initialized in SimulationContext.__init__(), but several unit tests create context objects via object.__new__(SimulationContext) (bypassing __init__). The _make_context_with_settings helper was updated to set _pending_camera_view = None and _render_generation = 0, but the simpler _make_context() helper was not. This causes the AttributeError that @kellyguo11 reported in CI:

E   AttributeError: 'SimulationContext' object has no attribute '_pending_camera_view'

The _make_context() function (test line ~130) also needs _pending_camera_view = None and _render_generation = 0 set on the context object, just as _make_context_with_settings() does.

🔴 Critical: _resolve_cfg_camera_pose uses bare getattr without fallback — base_visualizer.py:196-198

eye = tuple(float(v) for v in getattr(self.cfg, "eye"))
lookat = tuple(float(v) for v in getattr(self.cfg, "lookat"))

getattr(obj, name) without a default raises AttributeError if the attribute is missing — identical to self.cfg.eye. The getattr wrapping provides no safety benefit here and is misleading. If the intent is to support subclasses with different field names, add a default and handle the missing case. If eye/lookat are always present (they are, on VisualizerCfg), use direct attribute access for clarity:

eye = tuple(float(v) for v in self.cfg.eye)
lookat = tuple(float(v) for v in self.cfg.lookat)

The del visualizer_name is also a code smell — if the parameter is unused, either drop it from the signature or document why it exists for subclass overrides.

🟡 Warning: _viewer_cfg_value_matches_default fragile for nested configclass fields — envs/common.py:22-29

The deprecation check in ViewerCfg.__post_init__ compares each field to its declared default. This works for simple types but will silently miss nested @configclass fields whose defaults are factory-constructed — f.default would be MISSING and f.default_factory is a dataclasses concept that @configclass may not always set identically. For now all ViewerCfg fields are simple scalars/tuples so this is safe, but the approach should be noted as fragile if fields are added later.

🟡 Warning: pump_kit_app_for_headless_video_render_if_needed catches all exceptions silently — isaac_rtx_renderer_utils.py:138-139

    except Exception as exc:
        logger.debug("[isaac_rtx] Skipping Kit app-loop pump in render(): %s", exc)

A bare except Exception with only debug-level logging around ensure_isaac_rtx_render_update() means that genuine rendering failures (e.g., GPU resource exhaustion, CUDA errors) will be silently swallowed. At minimum, this should log at warning level so operators can diagnose headless video recording failures. The debug level is appropriate for expected non-Kit environments, but unexpected errors deserve more visibility.

🟡 Warning: Breaking change in camera config field names not documented in CHANGELOG — visualizer_cfg.py

Renaming camera_position→eye, camera_target→camera_target, camera_source→cam_source, camera_usd_path→cam_prim_path is a breaking change for any user with custom VisualizerCfg in their training scripts. The PR description mentions the rename but per Isaac Lab contributing guidelines, breaking changes must be documented in CHANGELOG.rst. No CHANGELOG update is included.

Additionally, the old → new error path changed from a warning + fallback (graceful) to a RuntimeError (crash) when cam_source='prim_path' fails to resolve. This is a stricter behavior change that should be called out:

# Before: warning + fallback to cfg camera
logger.warning("[NewtonVisualizer] camera_usd_path '%s' not found; using configured camera.", ...)
# After: hard crash
raise RuntimeError("[NewtonVisualizer] cam_source='prim_path' requires a resolvable camera prim path...")

While fail-fast is generally better for misconfigurations, this is a behavior change for existing users whose USD camera paths resolve at runtime (e.g., deferred scene loading).

🟡 Warning: KitVisualizerCfg.create_viewport default changed from False to True — kit_visualizer_cfg.py

This is a silent behavior change: all KitVisualizerCfg() users who relied on the default create_viewport=False will now get a new viewport created automatically. Combined with the new validation that viewport_name must be non-empty when create_viewport=True, this could surprise existing users.

🔵 Suggestion: Prebundle repoint search across symlinks could be slow — cli/commands/install.py:358-374

The new prebundle discovery resolves symlinks and searches up to 6+ root directories with rglob("pip_prebundle"). On systems with large Omniverse caches (e.g., ~/.local/share/ov/data/exts with hundreds of extensions), this could be noticeably slow. Consider caching the result or adding a debug-level log of how many dirs were scanned and how long it took, to aid diagnosis of slow isaaclab install runs.

🔵 Suggestion: Test module-level simulation_app launch prevents test isolation — test_visualizer_cartpole_integration.py:25

simulation_app = AppLauncher(headless=True, enable_cameras=True).app

Launching the Kit app at module import time means all tests in this file share a single app instance. This is standard for Isaac Lab tests, but worth noting that test ordering could matter — a test that corrupts app state (e.g., a failed viewport creation) could cascade failures to subsequent tests.

Test Coverage

• Bug fixes: Partial regression coverage. The Newton pause button fix has a unit test (test_update_visualizers_skips_zero_dt_for_paused_app_pumping_visualizer). The RTX stale image fix is exercised via the Kit viewport motion test. However, the headless recording fix and rerun+newton launch fix don't have dedicated regression tests — they're implicitly covered by the integration tests working, but a reintroduction of the bug might not be caught.
• New integration tests are comprehensive. The cartpole-based tests verify non-black frames, frame-to-frame motion detection (early vs late captures), and log hygiene across 4 visualizer × 2 backend combinations. The motion threshold approach (channel diff ≥ 50, ≥ 100 differing pixels) is well-calibrated for catching frozen/stale rendering.
• Deleted smoke tests replaced adequately. The old test_visualizer_smoke_logs.py tested initialization + stepping with a minimal env. The new tests use a real cartpole scene which is strictly more rigorous.
• Gap: No test for ViewerCfg deprecation warning. The __post_init__ deprecation path is only tested indirectly via test_configclass.py wrapping an existing test in pytest.warns. A direct test that ViewerCfg(eye=(1,1,1)) emits the warning while ViewerCfg() does not would be valuable.

CI Status

All checks passing ✅ (pre-commit, license-check, build wheel, docs, installation tests). Note: the CI failure @kellyguo11 reported (AttributeError: '_pending_camera_view') may have been from a prior commit — the current SHA adds the attribute in _make_context_with_settings but not in _make_context, so the issue likely persists for the test_non_explicit_create_failure_silently_logged test path.

Verdict

Significant concerns

The architecture and direction are right, but there are blocking issues: a CI-failing AttributeError from incomplete test helper updates, a breaking config rename without CHANGELOG documentation, and maintainer questions about SimulationContext backend coupling that need resolution. The new integration tests are high quality and a clear improvement. Recommend addressing the CI failure and maintainer feedback before merging.

🤖 Isaac Lab Review Bot — PR #5103

Summary

This PR delivers a substantial overhaul of the visualizer subsystem:

1. Camera config rename: camera_position/camera_target → eye/lookat, camera_source/camera_usd_path → cam_source/cam_prim_path — consistent API across all visualizers.
2. ViewerCfg deprecation: __post_init__ warning when non-default fields are overridden, migrating toward KitVisualizerCfg/NewtonVisualizerCfg as the sole camera config surface.
3. Bug fixes: Pause-rendering button in Newton, headless video recording in Kit, stale RTX image after resets, USD stage traversal safety, wrist camera orientation offset.
4. New render lifecycle hooks: after_visualizers_render() on PhysicsManager, render_generation counter, _pending_camera_view replay.
5. Kit visualizer: Dedicated camera prim (enable_visualizer_cam, visualizer_camera_prim_path, focal_length), viewport decoupling from sensor cameras.
6. Prebundle repointing: Expanded to Omniverse cache roots and new packages (websockets, viser, imgui_bundle).
7. Tests: Replaced smoke test with comprehensive cartpole integration tests including non-black frame and motion assertions.

Overall this is a well-structured PR with clear separation of concerns. Inline comments below cover specific issues found.

Verdict

🟡 Good with minor issues — A few robustness and correctness items to address before merge (see inline comments).

🤖 Isaac Lab Review Bot

Summary

This PR makes substantive improvements to the visualizer subsystem: renames camera_position/camera_target → eye/lookat across configs, fixes headless video recording by routing Kit app-loop pumping through PhysicsManager.after_visualizers_render(), adds a ViewerCfg deprecation path, decouples viewport camera control from visualizer broadcasting, and replaces the deleted smoke tests with comprehensive cartpole integration tests that verify non-black frames and motion detection.

The architecture changes are well-motivated — separating Kit-specific rendering from the backend-agnostic SimulationContext, adding render_generation to fix stale-frame dedup, and introducing pumps_app_update() to avoid double-pumping. A few issues are worth addressing before merge.

Design Assessment

Design is sound. The separation of Kit-specific rendering into isaaclab_physx (via kit_viewport_utils.py and after_visualizers_render) keeps SimulationContext backend-agnostic. The pumps_app_update() protocol is a clean way to prevent double-rendering. The ViewerCfg deprecation with __post_init__ field-diffing is thorough. The new integration tests are a significant quality improvement over the deleted smoke tests.

Minor concerns detailed in inline comments.

Test Coverage

• Bug fix regression tests: ✅ test_render_pumps_app_update_without_visualizer and test_render_skips_app_update_when_visualizer_pumps_it directly cover the headless video fix from PR #5056
• Integration tests: ✅ Comprehensive cartpole integration tests cover Kit, Newton, Rerun, and Viser visualizers with non-black frame and motion-detection assertions
• Deprecation warning: ✅ test_config_update_via_constructor updated to expect DeprecationWarning
• Paused visualizer behavior: ✅ test_update_visualizers_skips_zero_dt_for_paused_app_pumping_visualizer covers the new paused-but-pumping path
• Gap: No unit test for ViewerCfg.__post_init__ deprecation warning edge cases in isolation (e.g. list vs tuple equivalence in _viewer_cfg_value_matches_default)

CI Status

⚠️ Multiple CI checks failing — appears to be a Docker base image build failure cascading to downstream test suites, not caused by this PR's changes. pre-commit and Build Wheel pass.

Verdict

Minor fixes needed

The approach is correct and well-architected. The inline comments identify a few issues — mostly around error handling robustness and a cross-module dependency concern. None are blocking correctness for the primary use cases.